[CS2113-T15-1] Jikan #28

siuhian · 2020-03-03T07:29:58Z

@btricec
@nigellenl
@rdimaio
@ananda-lye

joelczk · 2020-03-30T08:21:34Z

docs/DeveloperGuide.md


-{Describe the design and implementation of the product. Use UML diagrams and short code snippets where applicable.}
+## 2. Design
+![image_info](./pictures/ClassDiagram.png)


The UML diagram describing the design of the program is too complicated. Perhaps it would be better to break the design down into smaller components to describe?
Also, your team could consider adding some description in words about the implementation so that it would be easier for the reader to understand the diagram.

joelczk · 2020-03-30T08:24:28Z

docs/DeveloperGuide.md

+
+Step 2. Read the first line of the status file, if a character ‘0’ is found, deactivate the automated cleanup. Else if a character ‘1’ is found, activate the automated cleanup.
+
+![image_info](./pictures/FlowchartinitCleaner.png)


The lines in the diagram are difficult to be seen. Perhaps it would be better to change the colour of the lines in the diagram so the diagram looks more visible for the reader?
Also, your team could consider adding in notes by the side of the diagram to indicate the meaning of "0" and "1" at the side of the diagram to allow readers to have a better understanding

Alternatively, you could also change the background theme of your Developer Guide to a lighter colour scheme. Just a recommendation, would it be better for you to use a boolean "True" and "False" instead of "1" and "0"

joelczk · 2020-03-30T08:28:12Z

docs/DeveloperGuide.md

+- Constructing a Storage object via `Storage(String dataFilePath)`, which takes in the path to the desired data file (or the path where the user wants to create the data file) as a String object.
+- Creating a data file via `createDataFile`.
+- Writing to a data file via `writeToFile`. This function takes a single string as parameter and writes it to the data file. It is recommended to only pass single-line strings to keep the file nicely formatted. 
+Loading a pre-existing data file via `loadFile`. If a data file already exists for the provided data file path, the function will return `true`; if the specified data file did not previously exist, this function will call the `createDataFile` method and returns `false`. The return value is useful so that the application knows whether or not this is the first session with a specific data file or if data already exists.


This portion is very lengthy, which makes it very confusing to understand. Perhaps you could break down this portion into smaller parts?
You could also consider adding sequence diagrams showing the higher-level flow of the Storage class to help readers understand the flow of Storage class

joelczk · 2020-03-30T08:29:27Z

docs/DeveloperGuide.md

+
+#### 3.4.1 Current Implementation
+The following sequence diagram shows how the edit feature works.
+![image_info](./pictures/EditSequenceDiagram.png)


Similiar to the comment that I gave above, the lines in the diagram are difficult to be seen. Perhaps it would be better to change the colour of the lines in the diagram so that the diagram looks more visible for the reader?

joelczk · 2020-03-30T08:32:33Z

docs/DeveloperGuide.md

+        * Else, it will respond to the user that there are no tasks which match the given keyword.
+
+
+![filter seq diagram](https://imgur.com/hybT3R9.png)


The diagram that is drawn may be too complicated for the readers as there are many "Loop" and "opt" between the Parser class and the FilterCommand class. Perhaps, you could consider breaking down the diagrams to show smaller components so that it is easier to understand?

joelczk · 2020-03-30T08:34:50Z

docs/DeveloperGuide.md

-{Give instructions on how to do a manual product testing e.g., how to load sample data to be used for testing}
+### Instructions for Manual Testing  
+
+  #### Launch and Shutdown


The Launch and Shutdown is not formatted properly. Perhaps, you have added in an extra space?

joelczk · 2020-03-30T08:42:53Z

docs/DeveloperGuide.md

+
+Step 2b. If the attribute toClean is equal to true, access the storage data file and remove some of the activities starting from the oldest. Put these deleted activities into the data file under the ‘recycled’ folder.
+
+![image_info](./pictures/SDautoClean.png)


The dotted lines from the Storage Class and StorageCleaner class extends beyond the cross symbol. Perhaps you could change the dotted lines to end at the cross symbol since the classes would already have been deleted by then?

jinfayap · 2020-03-30T08:26:24Z

docs/DeveloperGuide.md

+The continue feature allows the user to continue a previously ended activity.
+
+#### 3.5.1 Current Implementation
+![Continue command sequence diagram](./pictures/continueActivity.png)


Is the entities labelled correctly? Should it be not underlined?

Is there a missing return arrow from the Parser?

Is the order of getTags() and now() in the wrong order from looking at the code?

Is using opt correct? should it be alt? Since you used if else statement in your code.

Is there issue with the lifeline not attached to the activation bar in ContinueCommand.

jinfayap · 2020-03-30T08:44:23Z

docs/DeveloperGuide.md

+ 2. Gets the *name* and *tags* of the activity to be continued and saves it to a public static variable of *Parser* object
+ 3. Gets the current time and saves it to a public static variable of *Parser* object
+
+ ![End command sequence diagram](./pictures/endActivity.PNG)


Is the entities labelled correctly? Should it be not underlined?

Should the parser use a activation bar instead of line to make it more consistent for the user?

Is there an error with a line in the middle of EndCommand at the left hand side?

From code based has if else, should you use alt instead of opt?

Missing return arrow from updateDuration.

Similar issue with the lifeline not attached to the activation bar in EndCommand.

Is the text at the line of executeCommand() a meaningful one?

jinfayap · 2020-03-30T08:52:31Z

docs/DeveloperGuide.md

+
+#### 3.4.1 Current Implementation
+The following sequence diagram shows how the edit feature works.
+![image_info](./pictures/EditSequenceDiagram.png)


Is the entities labelled correctly? Should it be not underlined?

Should the parser use a activation bar instead of line to make it more consistent for the user?

Is there an error with a line in the middle of EditCommand at the left hand side?

should it be dashed arrow instead of solid line arrow at get()?

should the return arrow for updateName() be at the end.

should the activity have a up arrow?

Similarly, you can refer to https://nus-cs2113-ay1920s2.github.io/website/se-book-adapted/chapters/uml.html#sequence-diagrams

jinfayap · 2020-03-30T09:00:19Z

docs/DeveloperGuide.md

+
+Step 2b. If the attribute toClean is equal to true, access the storage data file and remove some of the activities starting from the oldest. Put these deleted activities into the data file under the ‘recycled’ folder.
+
+![image_info](./pictures/SDautoClean.png)


Is the entities labelled correctly? Should it be not underlined?

should you use alt instead of alternative?

should object deletion still have life line extended after the X?

Creation of storage constructor, activation bar not sticked to the entity.

Should the return arrow return halfway of the activation bar in the alt? Perhaps extending the activation bar then have it return.

You can refer to https://nus-cs2113-ay1920s2.github.io/website/se-book-adapted/chapters/uml.html#sequence-diagrams

Please refer to the sequence diagram screenshot as shown below.

Under the blue arrow, StorageCleaner is initialized, from the CallerClass, by calling its constructor. My question is, should there be a name before the semicolon? For example: someName:StorageCleaner

NizarMohd

Overall, the DG was pretty well done. As mentioned by other reviewers, you may want to consider either using a brighter background or brighter arrows as some diagrams may not be very visible because the arrows and the backgrounds are of the same colour. All in all, good job!

NizarMohd · 2020-03-31T03:33:05Z

docs/DeveloperGuide.md


-{Describe the design and implementation of the product. Use UML diagrams and short code snippets where applicable.}
+## 2. Design
+![image_info](./pictures/ClassDiagram.png)


As mentioned above, the UML diagram is pretty large. Aside from breaking down the UML into components, perhaps you might want to look into attributes in certain classes that have already been shown with an association? For example, you have the Parser class associated with StorageCleaner, and at the same time StorageCleaner is an attribute listed in the Parser Class. Perhaps you can consider omitting either the association or the attribute as mentioned in the 2113 website https://nus-cs2113-ay1920s2.github.io/website/se-book-adapted/chapters/uml.html#class-diagrams.

NizarMohd · 2020-03-31T03:42:18Z

docs/DeveloperGuide.md

+
+Step 2b. If ‘status’ is equal to false, deactivate the automated cleanup and write the character ‘0’ to the first line of the status file.
+
+![image_info](./pictures/FlowchartsetStatus.png)


Good use of activity diagram to explain the control flow

NizarMohd · 2020-03-31T03:45:27Z

docs/DeveloperGuide.md


 {Give non-functional requirements}

-## Glossary
+### Glossary


Perhaps you can use the explanation of crucial methods here, and add a clickable to direct the readers here when they stumble upon the usage of such methods in the DG. This may be a good way to help reduce the content in your implementation and make it look more succinct.

NizarMohd · 2020-03-31T03:48:09Z

docs/DeveloperGuide.md

@@ -1,34 +1,283 @@
 # Developer Guide



Perhaps you can add a table of contents and employ clickable(s) to make the DG more navigable.

You can also add in an introduction to your Development Guide that includes purpose of documentation, intended reader, brief description of your product.

benchan911

Looking forward to seeing your product.

benchan911 · 2020-03-31T04:25:46Z

docs/DeveloperGuide.md

@@ -1,34 +1,283 @@
 # Developer Guide



You can also add in an introduction to your Development Guide that includes purpose of documentation, intended reader, brief description of your product.

benchan911 · 2020-03-31T04:31:07Z

docs/DeveloperGuide.md


-{Describe the design and implementation of the product. Use UML diagrams and short code snippets where applicable.}
+## 2. Design
+![image_info](./pictures/ClassDiagram.png)


To add on to the above, it is not only not a requirement, but also not recommended by the professor to state all attributes and methods for a class diagram. You are recommended to include important details that will benefit the reader when they read your development guide.

It will also be clearer to include a brief explanation for you Fig 2.1. Class diagram of the Jikan program, to guide the reader through your UML diagram.

TA here.

General rule of thumb: More important for the diagram to be comprehensible than comprehensive.

@damithc

benchan911 · 2020-03-31T04:33:48Z

docs/DeveloperGuide.md

+
+Step 2. Read the first line of the status file, if a character ‘0’ is found, deactivate the automated cleanup. Else if a character ‘1’ is found, activate the automated cleanup.
+
+![image_info](./pictures/FlowchartinitCleaner.png)


Alternatively, you could also change the background theme of your Developer Guide to a lighter colour scheme. Just a recommendation, would it be better for you to use a boolean "True" and "False" instead of "1" and "0"

benchan911 · 2020-03-31T04:37:54Z

docs/DeveloperGuide.md

+
+Step 2b. If the attribute toClean is equal to true, access the storage data file and remove some of the activities starting from the oldest. Put these deleted activities into the data file under the ‘recycled’ folder.
+
+![image_info](./pictures/SDautoClean.png)


You can refer to https://nus-cs2113-ay1920s2.github.io/website/se-book-adapted/chapters/uml.html#sequence-diagrams

benchan911 · 2020-03-31T04:38:30Z

docs/DeveloperGuide.md

+
+#### 3.4.1 Current Implementation
+The following sequence diagram shows how the edit feature works.
+![image_info](./pictures/EditSequenceDiagram.png)


Similarly, you can refer to https://nus-cs2113-ay1920s2.github.io/website/se-book-adapted/chapters/uml.html#sequence-diagrams

benchan911 · 2020-03-31T04:44:08Z

docs/DeveloperGuide.md

+        * Else, it will respond to the user that there are no tasks which match the given keyword.
+
+
+![find seq diagram](https://imgur.com/Icg5rdB.png)


This is a good use of sequence diagram. Perhaps you can reference from this when making future changes.

benchan911 · 2020-03-31T04:47:16Z

docs/DeveloperGuide.md

+
+**Execution:**
+ - Continue by activity name (current implementation)
+ **Cons:** Activity names have to be unique.


I feel that there is a minor formatting error here. It will be clearer if you can break it into different lines instead of a single line.

JustinnT · 2020-03-31T04:43:10Z

docs/DeveloperGuide.md

@@ -1,34 +1,283 @@
 # Developer Guide


Agree with Nizar, perhaps providing Table of Contents can improve navigability

JustinnT · 2020-03-31T04:59:31Z

docs/DeveloperGuide.md


-{Describe the design and implementation of the product. Use UML diagrams and short code snippets where applicable.}
+## 2. Design
+![image_info](./pictures/ClassDiagram.png)


For class diagram, I think there should not be a semicolon before the class name.

According to the website linked here, under Object vs Class diagram, it says that There is a : before the class name for object diagram. Since this is a class diagram, then the semi colon should be omitted.

For your convenience, I have attached a snapshot of the aforementioned section~

JustinnT · 2020-03-31T05:06:29Z

docs/DeveloperGuide.md

+
+##### initialiseCleaner
+
+Step 1. Loads up the status file for reading. If the status file is not found, create a new status file and write the character ‘0’ to the first line of the status file.


Instead of using step 1. step 2. etc. to denote the steps of initializingCleaner, would it is better to use Markdown's available numbered list? This might improve reader visual as Markdown's default number list provides some indentation.

For example: we can write it like this.

initialiseCleaner

This section demonstrates the steps when initializing the cleaner.

Loads up the status file .....

Read the first line ....

JustinnT · 2020-03-31T05:11:32Z

docs/DeveloperGuide.md

+
+##### setStatus
+
+Step 1. Read the boolean parameter ‘status’.


In addition to my comment above regarding using Markup default numbering list, to have a "nested-list" to deal with step 2a and step 2b.

JustinnT · 2020-03-31T05:22:39Z

docs/DeveloperGuide.md

+
+Step 2b. If the attribute toClean is equal to true, access the storage data file and remove some of the activities starting from the oldest. Put these deleted activities into the data file under the ‘recycled’ folder.
+
+![image_info](./pictures/SDautoClean.png)


Please refer to the sequence diagram screenshot as shown below.

Under the blue arrow, StorageCleaner is initialized, from the CallerClass, by calling its constructor. My question is, should there be a name before the semicolon? For example: someName:StorageCleaner

JustinnT · 2020-03-31T05:47:04Z

docs/DeveloperGuide.md

+        * Else, it will respond to the user that there are no tasks which match the given keyword.
+
+
+![filter seq diagram](https://imgur.com/hybT3R9.png)


Please refer to this screenshot for the issue below

For exeucteCommad(), I believe the arrow should point towards the top of the activation bar

===============================================================

Please refer to the below screenshot for the issue below

Should the dotted line (as indicated by the red arrow) be a solid line, since it is a method call?

Add delete tag goal and fix update tag goal exception handling

…into SiuHian-DebugStart

Siu hian debug start

More refactoring

…into SiuHian-DebugStart

Change targets to allocation

# Conflicts: # docs/DeveloperGuide.md # docs/pictures/continue.png # docs/pictures/end.png # docs/pictures/graph.png

Update DG list and graph

Modified cleaners

Add Storage and StorageHandler assertions

Fix some format issues

Change diagram for DG

Update UG diagrams

edit UG

Fix some bugs

Remove redundant lines

Update UG with missing instruction

Update DG with missing instruction

Update section numberings in UG and DG

Fix test

joelczk reviewed Mar 30, 2020

View reviewed changes

jinfayap reviewed Mar 30, 2020

View reviewed changes

NizarMohd reviewed Mar 31, 2020

View reviewed changes

benchan911 reviewed Mar 31, 2020

View reviewed changes

JustinnT reviewed Mar 31, 2020

View reviewed changes

siuhian and others added 25 commits April 6, 2020 00:39

Can get activity name

c434639

Add delete for tag goals & fix exception handling for update tag goals

178eed5

Update text-ui

d9fdfb4

Fix text-ui

ddc22cb

Merge pull request #198 from nigellenl/master

da26f18

Add delete tag goal and fix update tag goal exception handling

Debugged start command

6b132a0

Merge branch 'master' of https://github.com/AY1920S2-CS2113-T15-1/tp …

f2f31b4

…into SiuHian-DebugStart

Merge pull request #199 from siuhian/SiuHian-DebugStart

8dd4b3c

Siu hian debug start

Made command class abstract

0f25184

Change saveActivity() parameters

d845a1f

Change graph to lastshownlist

de36baa

Merge pull request #200 from btricec/more_refactoring

a963e8c

More refactoring

Standardize the use of allocation

df6d5d1

Change targets to allocation

b51804d

Merge branch 'master' of https://github.com/AY1920S2-CS2113-T15-1/tp …

e2d0ff1

…into SiuHian-DebugStart

Merge pull request #201 from siuhian/SiuHian-DebugStart

3e570b3

Change targets to allocation

Update DG list and graph

1e0224b

Merge branch 'master' of https://github.com/AY1920S2-CS2113-T15-1/tp

8d56eee

# Conflicts: # docs/DeveloperGuide.md # docs/pictures/continue.png # docs/pictures/end.png # docs/pictures/graph.png

Merge pull request #202 from btricec/master

50a708e

Update DG list and graph

Create abstract cleaner class

31af75f

Modify both cleaners

95853fe

Merge pull request #203 from siuhian/SiuHian-DeleteByBatch

1bbe072

Modified cleaners

Add assertions

596ad5b

Merge pull request #204 from rdimaio/master

9de90a7

Add Storage and StorageHandler assertions

Add more assertions

e6b34ee

siuhian and others added 30 commits April 11, 2020 19:37

Merge pull request #268 from siuhian/master

9c00f28

Fix some format issues

Change diagram for DG

3816162

Merge pull request #269 from siuhian/master

b32829d

Change diagram for DG

Update UG diagrams

df45057

Merge pull request #270 from siuhian/master

39f081e

Update UG diagrams

Update FindCommand.java

9b1ceee

Fix some bugs

e183381

edit UG

b3865b6

Merge pull request #271 from siuhian/master

a4e449e

edit UG

Fix style

10c276f

Merge pull request #272 from btricec/master

d92c1ca

Fix some bugs

Update UserGuide.md

f2f2757

Update UserGuide.md

6398b8a

Update UserGuide.md

52d4543

Update UserGuide.md

ba40a49

Update UserGuide.md

9dd2d41

Remove redundant lines

b482f70

Merge pull request #273 from btricec/master

b235f47

Remove redundant lines

Update UG with missing instructions

c53d6a2

Merge pull request #274 from nigellenl/master

fcd37c9

Update UG with missing instruction

Update DG with missing instruction

4c2a44c

Merge pull request #275 from nigellenl/master

d8b2862

Update DG with missing instruction

Update DG numbering

373f1bb

Update UG numbering

3d68084

Merge pull request #276 from rdimaio/master

cbced33

Update section numberings in UG and DG

Fix test

421f778

Catch exceptions in test

f452667

Fix test

ce08e50

fix test

2c69397

Merge pull request #277 from rdimaio/master

d9eb7b0

Fix test


		Step 2. Read the first line of the status file, if a character ‘0’ is found, deactivate the automated cleanup. Else if a character ‘1’ is found, activate the automated cleanup.

		![image_info](./pictures/FlowchartinitCleaner.png)

		* Else, it will respond to the user that there are no tasks which match the given keyword.


		![filter seq diagram](https://imgur.com/hybT3R9.png)


		Step 2b. If the attribute toClean is equal to true, access the storage data file and remove some of the activities starting from the oldest. Put these deleted activities into the data file under the ‘recycled’ folder.

		![image_info](./pictures/SDautoClean.png)


		Step 2b. If ‘status’ is equal to false, deactivate the automated cleanup and write the character ‘0’ to the first line of the status file.

		![image_info](./pictures/FlowchartsetStatus.png)

		* Else, it will respond to the user that there are no tasks which match the given keyword.


		![find seq diagram](https://imgur.com/Icg5rdB.png)


		##### initialiseCleaner

		Step 1. Loads up the status file for reading. If the status file is not found, create a new status file and write the character ‘0’ to the first line of the status file.


		##### setStatus

		Step 1. Read the boolean parameter ‘status’.

[CS2113-T15-1] Jikan #28

Are you sure you want to change the base?

[CS2113-T15-1] Jikan #28

Conversation

siuhian commented Mar 3, 2020

joelczk Mar 30, 2020 • edited Loading

Choose a reason for hiding this comment

joelczk Mar 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NizarMohd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benchan911 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

initialiseCleaner

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joelczk Mar 30, 2020 •

edited

Loading

joelczk Mar 30, 2020 •

edited

Loading